fix(sdk): declare typescript and undici as runtime dependencies so flows check works on a clean install - #608
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Relayflow: the adversarial review did not pass. This branch is not approved: the flow stopped here and did not mark it ready to merge. Review of PR #608Reviewed head: The runtime dependency fix is correct on inspection, and the requested clean-install regression reproduces the original crash and passes after restoration. No confirmed implementation defect found. Review remains open for verification and reporting issues; Open items
Verification performed in this reviewEach linked transcript contains literal commands, captured output and exit statuses.
Diff and edge-case assessmentThe manifest preserves the TypeScript range, the npm lock removes its dev-only marker, and the added undici declaration matches an existing runtime import. No SDK implementation or workflow changes are included. The test checks both bin owners explicitly, rejects ancestor node_modules, clears Node resolution overrides, omits unrelated optional runtime binaries, and checks that TypeScript does not expose global compiler bins. Network errors fail rather than silently skip; the explicit skip variable reports packaging as unverified. The AST scan documents its computed-import blind spot. The packaging test requires built surface/SDK artifacts and Node >=22.18.0; existing CI builds those artifacts before running the SDK suite. Standalone binaries, older Node versions and Windows packaging are not established by these Unix-layout checks. Those are coverage limits, not newly demonstrated product regressions. PR commentsRead all available conversation comments, review records and inline review comments for PR #608. The only conversation comment is CodeRabbit's skipped-review notice; reviews and inline review comments are empty. There are no substantive reviewer findings to resolve. See the exact responses in pr.txt. No implementation or judging-gate changes were made during this review. Existing untracked |
Session-Id: 570354ad-8fe1-4ec2-b442-d7bea7551ff7
Session-Id: 570354ad-8fe1-4ec2-b442-d7bea7551ff7
Session-Id: 570354ad-8fe1-4ec2-b442-d7bea7551ff7
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Session-Id: 570354ad-8fe1-4ec2-b442-d7bea7551ff7
|
The review's open items (unverified full suite vs base, committed evidence files, PR description accuracy) are being finished by fleet agent |
a56ce50 to
812000a
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 812000a. Configure here.
|
Addressing the adversarial review's open items (head
Cursor Bugbot: no new issues. Leaving the merge to a human. |
* ci: run the CLI packaging gate when its own inputs change #608 added scripts/cli-package-gate.mjs (run by packages/sdk/tests/cli-package-gate.test.ts) to catch a runtime dependency missing from the published CLI. The workflows that run that test only trigger on pull requests touching packages/sdk/** (and their own paths), so a PR that changes only the gate script or packages/relayflows/** (the `relayflows` wrapper package, whose dependency on @relayflows/sdk the gate installs) skipped it. Add both paths to the pull_request filters of cloud-runtime-artifact.yml and to both filters of surface-package.yml. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ci: list every CLI packaging-gate input in the artifact workflow's filter Review on #610 (Devin, Codex): the gate also runs scripts/pack-release.mjs, packs packages/surface/**, and checks examples/dependency-upgrade-bot/dependency-upgrade-bot.flow.ts, so a PR touching only one of those still skipped it. And the gate only runs in cloud-runtime-artifact.yml's full vitest run; surface-package.yml runs tests/authored-flow.test.ts, so adding the paths there gated nothing. Put all gate inputs in cloud-runtime-artifact.yml's pull_request filter, with a comment saying why, and drop the surface-package.yml change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Relayflow Lead <lead@relayflows.local> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes #605.
flows checkcrashes on a clean install (ERR_MODULE_NOT_FOUND: typescript) because@relayflows/sdkimportstypescriptat runtime (src/cli/check-activities.ts) but only declared it as a devDependency.Change
packages/sdk/package.json/package-lock.json: movetypescript(^5.6.0, range unchanged) todependencies, and declareundici(^7.29.1), which the SDK already imports at runtime and was likewise only resolving transitively. Lock regenerated without upgrades: TypeScript loses itsdev: truemarker; there is one undici entry. TypeScript, not acorn, is needed because the activity checker parses TypeScript syntax.packages/sdk/tests/runtime-dependencies.test.ts: AST-scans the built SDK JavaScript for literal imports / re-exports /requirecalls and fails on any bare package not declared independencies/peerDependencies. Computed imports are a documented blind spot.packages/sdk/tests/cli-package-gate.test.ts+scripts/cli-package-gate.mjs: the clean-install regression flows check crashes on a clean install: @relayflows/sdk imports typescript but only dev-depends on it #605 asked for. Packs surface, SDK andrelayflowswith the existingscripts/pack-release.mjs, installs the real tarballs into a temp dir outside the repo (it rejects any ancestornode_modulesand clearsNODE_PATH/NODE_OPTIONS), and runsflows checkonexamples/dependency-upgrade-botfor:flowsbin and therelayflowswrapper entry point. Both the SDK andrelayflowsclaim theflowsbin, so checking only the bin would leave the wrapper unexercised.flows(notsc/tsserver) and is checked against a consumer project that has no TypeScript of its own.It also asserts TypeScript resolves inside the installation. It needs Node >=22.18.0 and built surface/SDK
dist, which the CI SDK job already provides. Registry errors fail the test.FLOWS_SKIP_PACKAGE_GATE=1is an explicit, loudly reported local opt-out.tsconfig.tests.jsonincludes the two new tests.No workflow files change and no committed evidence ships; an earlier revision's
evidence/transcripts were removed.Verification
Packaging regression is red on base, green on head. I copied the two new tests and the gate script onto base
d1877301(main, typescript still a devDependency) and ranvitest run tests/runtime-dependencies.test.ts tests/cli-package-gate.test.tsin both trees:Full SDK suite: the PR adds no failures. CI's
linux-x64-artifactjob runs the whole SDK suite, including both new tests, and is green on812000a3. The first attempt hit two known main flakes, both seen on main:authored-parallel-agents"never starts queued agents once the body has failed" (main run 37137600293) and anamed-gate-diagnosticsword_count_bounds"could not run wc" spawn error (main run 37103010381). They passed on--failedrerun.Locally (macOS arm64, Node 26.8.1, bun 1.4.0, a freshly built
relayflowdviaRELAYFLOWD_BIN,RELAYFLOWS_ALLOW_ANALYZER_SKIP=1), I ran the full suite with an identical command and environment on head and on its based1877301, one after the other:vitest run --no-file-parallelism, after CI's setup steps (surface build,npm ci, local-surface override, typecheck, build, typecheck:tests).d1877301812000a3The sets of failing tests, by file and full name, are identical (
comm: 0 head-only, 0 base-only). The two extra passing files on head are the new tests. The 52 shared failures are environmental on this Mac:hosted-base-snapshot(15),hosted-extension-isolation(12),hosted-extension-protocol(7): "hosted extension isolation requires Linux" (bubblewrap sandbox).authored-node-runtime(14),cli(2),live-kernel(1),authored-parallel-agents(1): local authored-runtime/worker failures that also fail on base and pass in CI on Linux.Also:
npm run typecheck,npm run buildandnpm run typecheck:testsinpackages/sdkexit 0 on head.Not covered / follow-ups
cloud-runtime-artifact.ymltriggers the SDK job onpackages/sdk/**(among others). A future PR touching onlyscripts/cli-package-gate.mjs,scripts/pack-release.mjs,packages/surface/**,packages/relayflows/**or the example would not run this gate. Adding those paths is a one-line follow-up that needs aworkflow-scoped push; it was left out of this PR.enginesrange and the stale SDKbun.lockare outside this fix.🤖 Generated with Claude Code